Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duration features #1137

Closed
wants to merge 4 commits into from
Closed

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented Jun 8, 2023

If we drop time 0.1 compatibility in #1095 it would be nice if we could also take care of some of the wishlist items for Duration.

Three features that would be more involved and not part of this PR are:

Making the remaining methods const (#309, #638) would depend on raising the MSRV as in #1080.

Based on #1095.

@pitdicker
Copy link
Collaborator Author

Maybe we can fix the confusion with std::Duration and our current OldDuration name in the documentation?
Rename Duration to TimeDelta, and make Duration a type alias.

@pitdicker pitdicker force-pushed the duration_features branch 2 times, most recently from 40ee6f2 to 6da23f4 Compare June 8, 2023 17:17
@pitdicker pitdicker force-pushed the duration_features branch 2 times, most recently from 89e8b41 to ee4ebe3 Compare June 30, 2023 11:31
@jtmoon79
Copy link
Contributor

jtmoon79 commented Jul 9, 2023

Maybe we can fix the confusion with std::Duration and our current OldDuration name in the documentation? Rename Duration to TimeDelta, and make Duration a type alias.

Might be too late to chime in on this, but ChronoDuration is readily self-explanatory.

@pitdicker
Copy link
Collaborator Author

If a rename/alias to avoid confusion with the standard library is up for discussion:

  • ChonoDuration
  • TimeDelta (currently on main)
  • TimeSpan
  • Interval

@jtmoon79 I may not get the subtleties of the words completely. Is there one that is most appropriate for a value that can be positive and negative?

@jtmoon79
Copy link
Contributor

jtmoon79 commented Jul 9, 2023

If a rename/alias to avoid confusion with the standard library is up for discussion:

  • ChonoDuration
  • TimeDelta (currently on main)
  • TimeSpan
  • Interval

@jtmoon79 I may not get the subtleties of the words completely. Is there one that is most appropriate for a value that can be positive and negative?

I think all the terms proposed there are sensible for positive and negative values.

I strongly prefer ChronoDuration because it readily explains itself as:

  1. a Duration, similar to std::Duration
  2. from the chrono crate

Synonyms like TimeDelta, TimeSpan, etc. imply the object is very different from std::Duration, and may add difficulty for non-native English readers.

@fsktom
Copy link

fsktom commented Jul 25, 2023

I'm very excited about these quality of life improvements to the duration type! (esp. finally having the iter::Sum implementation)

I have to agree with @jtmoon79 on the new name for Duration.
ChronoDuration really seems like the most sensible option. The other two sound too scientific/abstract in my opinion and may really make it seem like it's a lot different from std::time::Duration.
I think it's a better name, even if it makes it a little bit more verbose and the following would look pretty weird (consecutive chrono):

let dur = chrono::ChronoDuration::zero();

Though I suppose most people would do use chrono::ChronoDuration if they're dealing with time spans anyway.

@pitdicker
Copy link
Collaborator Author

Added a commit to make the methods const.

@pitdicker pitdicker force-pushed the duration_features branch 2 times, most recently from ba8dae7 to 262bdf5 Compare July 27, 2023 14:44
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly prefer using alias ChronoDuration, e.g.

use crate::duration::Duration as ChronoDuration;

However, using just Duration is acceptable so long as it is consistent everywhere, e.g.

use crate::duration::Duration;

src/date.rs Outdated Show resolved Hide resolved
src/duration.rs Outdated
}

/// Makes a new `Duration` with given number of seconds.
/// Returns None when the duration is more than `i64::MAX` milliseconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: docstring referring to specific value None (None) should format it as code, i.e. add backticks

/// Returns `None` when the duration is ...

src/duration.rs Outdated
#[cfg(feature = "std")]
pub fn to_std(&self) -> Result<StdDuration, OutOfRangeError> {
if self.secs < 0 {
return Err(OutOfRangeError(()));
}
Ok(StdDuration::new(self.secs as u64, self.nanos as u32))
}

/// Returns true if and only if d < MIN || d > MAX
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: why use the mathematical proof phrase "if and only if" here? I suggest wording "if", i.e.

/// Returns `true` if d < MIN || d > MAX

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #1137 (ae3827c) into 0.4.x (0f19d6b) will increase coverage by 0.02%.
The diff coverage is 98.75%.

@@            Coverage Diff             @@
##            0.4.x    #1137      +/-   ##
==========================================
+ Coverage   91.55%   91.57%   +0.02%     
==========================================
  Files          38       38              
  Lines       17355    17385      +30     
==========================================
+ Hits        15889    15920      +31     
+ Misses       1466     1465       -1     
Files Coverage Δ
src/naive/time/mod.rs 96.55% <100.00%> (ø)
src/duration.rs 93.54% <98.71%> (+0.61%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@djc
Copy link
Member

djc commented Sep 8, 2023

Going to hold off on this for the next week, let's discuss again around Sep 18.

@pitdicker
Copy link
Collaborator Author

Would you like me to split up this PR?

@djc
Copy link
Member

djc commented Sep 21, 2023

Yes, let's try it. Please rate limit your submissions!

@pitdicker
Copy link
Collaborator Author

Haha. Okay.

@pitdicker
Copy link
Collaborator Author

With #1327 merged this now only has the changes to make methods on Duration const. Partially based on #638.

src/duration.rs Outdated
pub fn to_std(&self) -> Result<StdDuration, OutOfRangeError> {
if self.secs < 0 {
return Err(OutOfRangeError(()));
}
Ok(StdDuration::new(self.secs as u64, self.nanos as u32))
}

/// Returns `true` if d < MIN || d > MAX
const fn out_of_bounds(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a constructor, not a method on an already constructed type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely right!

Actually I wanted to have a Duration::new in #1310 and #1290.

@pitdicker
Copy link
Collaborator Author

The remainder of this PR has become part of #1337.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants